Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "speed" property to path nodes. #1424

Merged
merged 1 commit into from
Jun 18, 2020

Conversation

divVerent
Copy link
Contributor

To not break the current level format compatibility, this property is editor
only and the "time" property remains authoritative. However, whenever speed
is set, any editor action touching the node will update time according to speed
and the distance to the next node.

@tobbi
Copy link
Member

tobbi commented Jun 11, 2020

I gotta ask: Why is there a const iterator for next_node but not one for prev_node ?

@divVerent
Copy link
Contributor Author

divVerent commented Jun 11, 2020 via email

@tobbi
Copy link
Member

tobbi commented Jun 11, 2020

No, it's perfectly fine :) I was just wondering, that's all.

@divVerent
Copy link
Contributor Author

divVerent commented Jun 12, 2020

Actually, I just noticed that the mutable version of next_node is unused, so I'm gonna delete it. So previous node is only ever used mutably, next node only ever used immutably, mainly because the time for node A to node B is stored in node A.

@divVerent divVerent force-pushed the divVerent/node-speed branch from 2781a59 to 53c7869 Compare June 12, 2020 21:18
update_node_time(m_node, next_node());
}

std::vector<Path::Node>::iterator NodeMarker::prev_node() {
Copy link
Member

@Zwatotem Zwatotem Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me type name std::vector<Path::Node>::iterator seems a bit long and hard to read. It could be my lack of experience in bigger projects. Also I'm not sure, what is our policy regarding typedef.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a couple typedef's lying around the code, so we definitely use them.

@Zwatotem
Copy link
Member

I think you should change everything back to mutable, (or maybe that's not the cause) because I'm getting read access violation exception upon deleting nodes.

@divVerent
Copy link
Contributor Author

I think you should change everything back to mutable, (or maybe that's not the cause) because I'm getting read access violation exception upon deleting nodes.

I cannot reproduce; however I did find an unrelated bug in node deletion. Deleting the first node of a new (size 1) path TWICE crashes:

888	  for (auto i = m_edited_path->m_nodes.begin(); i != m_edited_path->m_nodes.end(); ++i) {
(gdb) print m_edited_path->m_nodes.size()
$9 = -1

Didn't know you can make a std::vector have negative size ;)

Anyway, can you explain how you caused the read access violation by deleting nodes? Like, something of the sort "make new path, add 5 nodes, then delete 3rd, then 4th, then 1st, then 1st"?

@divVerent
Copy link
Contributor Author

Sent PRs for all crashes I know (all pre-existing), including this one about node deletion I just found.

@Zwatotem
Copy link
Member

So as precisely, as possible:
(I was debugging on VS2019.) Open level editor, create any new level (or go to existing one). Add a flying platform object and select path object. Add new nodes to the path. Then change your tool to eraser, and select any of existing nodes.
However I just realized this doesn't occur on Release build.

@divVerent
Copy link
Contributor Author

I can't reproduce it that very way (but am on Linux/x86_64. Does it matter for you which node you delete?

Also, I am very sure the constness can't be at fault, as one would need a const_cast to cause any runtime problems with that. Can you maybe obtain a backtrace of the failure in VS2019?

@Zwatotem
Copy link
Member

Zwatotem commented Jun 15, 2020

No, the choice of deleted node didn't matter.

I can't provide you a backtrace anymore, since VS is making problems with building that I have no time to fix now. However I feel this is not a big problem, since on .msi from appveyor everything works smoothly (Except first node deletion)

Edit: I have couple more notes on how the thing should look like, (e.g. defaulting editor to insert new paths with constant speed, not time), but be the one to decide, if it's in scope of this PR or not.

@divVerent
Copy link
Contributor Author

I just managed to repro a crash: when the editor is in copy mode, and you use the erase tool on a node marker, then it crashes, at least with ASan enabled.

Of course, copy mode + erase tool itself is nonsense right now, as it makes a copy then erases it immediately - nevertheless this seems worth fixing.

@divVerent
Copy link
Contributor Author

Wait no - ANY erase of a platform triggers asan. In code unrelated to my change.

@divVerent divVerent force-pushed the divVerent/node-speed branch from d5a1f0d to 8c94c76 Compare June 15, 2020 16:13
To not break the current level format compatibility, this property is editor
only and the "time" property remains authoritative. However, whenever speed
is set, any editor action touching the node will update time according to speed
and the distance to the next node.
@divVerent divVerent force-pushed the divVerent/node-speed branch from 8c94c76 to fe79c27 Compare June 16, 2020 18:45
@divVerent
Copy link
Contributor Author

ASan fixed by bfd11ac - the crash that I did find fixed by b9cf4f3. Can you try again on Windows?

@Zwatotem
Copy link
Member

Okay, seems woking correctly. Merging.

@Zwatotem Zwatotem merged commit 207daac into SuperTux:master Jun 18, 2020
@divVerent divVerent deleted the divVerent/node-speed branch June 18, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants